Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ci] Run python-so-copying.yml on Linux and Mac both [WIP] #2208

Closed
wants to merge 2 commits into from

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Mar 5, 2024

Issue and/or context: My current theory on TileDB-Inc/tiledbsoma-feedstock#95 and TileDB-Inc/tiledbsoma-feedstock#91 is that this code path, not being regression-tested on MacOS, is perhaps faulty on MacOS. Regardless, the logic which was developed on #1937 needs testing on MacOS, and it was an oversight on #1937 for us to have only tested on Linux.

See also #2210 for tracking.

See also #2219 for tracking.

Changes:

Notes for Reviewer:

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Merging #2208 (df00bac) into main (05cf351) will decrease coverage by 5.56%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2208      +/-   ##
==========================================
- Coverage   77.68%   72.13%   -5.56%     
==========================================
  Files         137      103      -34     
  Lines       10679     6875    -3804     
  Branches      214      215       +1     
==========================================
- Hits         8296     4959    -3337     
+ Misses       2297     1818     -479     
- Partials       86       98      +12     
Flag Coverage Δ
libtiledbsoma 67.72% <ø> (+3.73%) ⬆️
python ?
r 74.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api ∅ <ø> (∅)
libtiledbsoma 48.85% <ø> (+0.15%) ⬆️

Copy link
Collaborator

@jdblischak jdblischak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started!

container:
image: ubuntu:22.04
image: ${{ matrix.os }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the tricky part. This needs to be a Docker image, not the name of the GitHub Actions runner.

The linux job runs inside of a Docker container (this made it easier for me to test locally). Thus it's not as easy as adding macos-12 to the build matrix. We need to either 1) make the container directive only apply to the ubuntu job (probably possible, but not immediately clear to me how), or 2) create a second job that runs similar code on macOS (which might be worth it given all the changes to accommodate running the same steps on linux and macOS)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdblischak I'm totally happy with well-documented code-duplication here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did find some workarounds to create a build matrix that builds natively on macOS but in Docker on Ubuntu. There is no if, so the key is to pass null to container for the macOS job. Overall the readability is not great though.

Also, I realized that we'll also need to download a different pre-built libtiledb binary for the macOS build, which makes the option to have a completely separate macOS build all the more appealing.

@johnkerl johnkerl changed the title [ci] Run python-so-copying.yml on Linux and Mac both [ci] Run python-so-copying.yml on Linux and Mac both [WIP] Mar 5, 2024
@johnkerl johnkerl closed this Mar 5, 2024
@johnkerl johnkerl deleted the kerl/debug-mac-dylib-copy branch March 15, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants